Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Replace default symfony error page with a shiny new Joomla one #30056

Merged
merged 1 commit into from
Apr 23, 2021
Merged

[4.0] Replace default symfony error page with a shiny new Joomla one #30056

merged 1 commit into from
Apr 23, 2021

Conversation

PhilETaylor
Copy link
Contributor

@PhilETaylor PhilETaylor commented Jul 8, 2020

Closes #29968

Summary of Changes

READY FOR TESTING

A new custom fatal error page to replace the default symfony error page.

Joomla 4 now uses symfony/error-handler component for handling fatal errors WHEN DEBUG MODE IS OFF (or in this case, when its not yet been turned on or off by JConfig)

This is NOT a replacement error page or a duplication of template/system/error.php. That file will continue to be used for all the reasons its used already. It cannot be used when a fatal error occurs on boot up (and doesnt in Joomla 4 before this PR) because it relies on Joomla being booted, working, and relies on the whole stack working.

fatal-error.html is displayed when things are REALLY bad and unrecoverable only. Like your database server being offline.

This page is only used during fatal unrecoverable errors, normally at boot time, when it cannot be assumed any of Joomla is working (certainly not a Factory or application running, certainly not a language object available)

Upstream Dependancy

It relies on upstream changes I made in the symfony/error-handler component that have been approved and merged, and that have been made specifically to support the ability of Joomla to render such a custom fatal page and Joomla will be one of the very first the first project to customise the Symfony default error page, when using symfony/error-handler as a component in another non-symfony stack.

Testing Instructions

Install Joomla 4 from 4.0-dev
merge/apply #31743 (running npm i or node build/build.js --build-pages to be sure)
apply this PR - run composer self-update --1 && composer update (with v1 of composer)
edit configuration.php and change class JConfig to be class JConfigBroken
Visit your home page.

Actual result BEFORE applying this Pull Request

image

Expected result AFTER applying this Pull Request

Screenshot 2020-12-20 at 21 48 39

Documentation Changes Required

New screenshots?

This page needs some content https://docs.joomla.org/J4.x:FatalError

templates/system/fatal.php Outdated Show resolved Hide resolved
<meta name="robots" content="noindex, nofollow">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>An Error Occurred: <?php echo $statusText; ?></title>
<style>body {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awkward in the sense that you're copy/pasting all this css. It can be done in a much more maintainable fashion, egg:

<style>
  <?php echo @file_get_conents(__DIR__ . '/../css/incompatible.css'); ?>
</style>

fwiw it will need a minor tweak so the css is copied from the build/... folder to the system template

This comment was marked as abuse.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember anymore the code that is there but if it is using CSS Custom properties all you have to do is jus redeclare them after echoing the css content

@PhilETaylor PhilETaylor marked this pull request as draft July 15, 2020 10:38
composer.json Outdated Show resolved Hide resolved
@wilsonge
Copy link
Contributor

wilsonge commented Aug 8, 2020

I like it but am going to wait for the 5.2 tagged version of symfony error handler to be out first! I don't think that will be any sort of issue.

@PhilETaylor

This comment was marked as abuse.

@wilsonge
Copy link
Contributor

wilsonge commented Aug 9, 2020

I don't have one yet. It may not cut in time. There's a loose aim to get a release out this year. But currently still at least a few more beta's I suspect before we're going to be able to cut a RC

@PhilETaylor

This comment was marked as abuse.

@wilsonge
Copy link
Contributor

wilsonge commented Oct 5, 2020

I'd be decently confident this will make it before we tag RC for what it's worth and where release blockers are right now

@ceford
Copy link
Contributor

ceford commented Nov 18, 2020

Note to others: do not test until it is declared ready to test. Otherwise just applying the patch causes a fatal error. To fix: remove the line added in diff libraries/bootstrap.php - line 57. Then revert the patch.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30056.

@PhilETaylor

This comment was marked as abuse.

@wilsonge
Copy link
Contributor

@PhilETaylor Looks like it's out https://symfony.com/blog/symfony-5-2-0-released - so whenever you're ready to get that in we can get this tested and in

@wilsonge
Copy link
Contributor

@PhilETaylor quick bump

@HLeithner
Copy link
Member

@PhilETaylor since I saved your sunday could you finish this PR please?

@PhilETaylor

This comment was marked as abuse.

@HLeithner
Copy link
Member

I would start with upgrading the branch, then changed composer dependencies to ^5.2 and have a look at the css yes ;-)

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor PhilETaylor marked this pull request as ready for review December 20, 2020 21:54
@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@wilsonge
Copy link
Contributor

No drone failure is sadly currently correct. They added a dependency here beberlei/assert#286 in the package your updating to 3.3.0. It got removed here beberlei/assert#289 in their 3.2.5 version. But as per beberlei/assert#309 it never made it into the 3.3 branch :(

@PhilETaylor

This comment was marked as abuse.

@wilsonge
Copy link
Contributor

Because you have the intl dependency on your macbook i think. But that's not a required Joomla dependency. It's not even a dependency for their library (aside from two documented functions - hence the re-removal). I've done a PR to their upstream to fix the issue. But remains on them to merge. beberlei/assert#310

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Feb 25, 2021

@PhilETaylor so I redid that PR as there was a major refactor of the build tools few days ago and there were a lot of conflicts.
One thing: as there were complains about moving the error pages to the media folder I reverted that part, so it means you have to revert it also here eg:

// Joomla supplied fatal error page
 if (file_exists(__DIR__ . '/../../templates/system/fatal-error.html'))
 {
 	$template = file_get_contents( __DIR__ . '/../../templates/system/fatal-error.html');
 }

BTW you have also conflicts in this PR

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Contributor

great news

@dgrammatiko
Copy link
Contributor

@PhilETaylor run composer install symfony/error-handler instead of just install

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

file headers and explinations

updating composer.lock for ci to work

cs

remove unused css

add Customised Fatal Error Template to .gitignore

Pin symfony/error-handler to ^5.2

Signed-off-by: Phil E. Taylor <phil@phil-taylor.com>

use fatal-error.html implemented in #31743

Signed-off-by: Phil E. Taylor <phil@phil-taylor.com>

Commit new composer.lock

Signed-off-by: Phil E. Taylor <phil@phil-taylor.com>

add all three placeholders for completeness

Signed-off-by: Phil E. Taylor <phil@phil-taylor.com>

Update templates/system/fatal.php

Co-authored-by: Dimitris Grammatikogiannis <d.grammatiko@gmail.com>

Update templates/system/fatal.php

Co-authored-by: Dimitris Grammatikogiannis <d.grammatiko@gmail.com>

Chane paths, custom filename, and order od loading

Signed-off-by: Phil E. Taylor <phil@phil-taylor.com>

 composer req symfony/error-handler

merge gitignore
@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor PhilETaylor marked this pull request as ready for review April 20, 2021 11:06
Copy link
Contributor

@wilsonge wilsonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - one quick test here and I'm happy to merge

@Quy
Copy link
Contributor

Quy commented Apr 21, 2021

I have tested this item ✅ successfully on 9c389a2


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30056.

@wilsonge wilsonge merged commit ed9d8e8 into joomla:4.0-dev Apr 23, 2021
@wilsonge
Copy link
Contributor

Thanks!

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor PhilETaylor deleted the fatalerror branch April 23, 2021 07:35
@richard67 richard67 added this to the Joomla 4.0 milestone Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.